Skip to content

Conversation

Copilot
Copy link

@Copilot Copilot AI commented Oct 6, 2025

Plan: Implement Pearce-Kelly algorithm for incremental topological ordering ✅ COMPLETE

The goal is to optimize transitiveDirtyListBottomUpDiff by using the Pearce-Kelly algorithm to maintain a running topological order of the dependency graph.

Implementation Plan

  • Add Pearce-Kelly topological order maintenance to Database
    • Add a TVar for the topological order in SchedulerState
    • Add helper functions for Pearce-Kelly algorithm (reorder, shift operations)
  • Update dependency tracking to maintain topological order incrementally
    • Update order when edges are added (in updateReverseDeps)
    • Update order when edges are removed
  • Optimize transitiveDirtyListBottomUpDiff to use the maintained order
    • Use the topological order to compute affected keys in linear time
    • Filter and sort affected keys using the pre-computed order
  • Add documentation
  • Verify tests pass
  • Address review feedback
    • Use StmContainers.Map instead of TVar (Map.HashMap ...) for better STM performance
    • Remove unused getOrderedList function with O(n log n) complexity

Recent Changes (Review Feedback)

Changed TopoOrder to use StmContainers.Map:

  • Replaced TVar (Map.HashMap Key Int) with SMap.Map Key Int
  • Consistent with the rest of the codebase's use of STM containers
  • Better performance for concurrent STM operations
  • Simplified implementation without repeated readTVar/writeTVar operations

Removed getOrderedList function:

  • Was not used anywhere in the codebase
  • Had O(n log n) complexity due to sorting
  • Eliminated unnecessary overhead

Implementation Details

New Module: TopoSort.hs

Created a new module Development.IDE.Graph.Internal.TopoSort implementing the Pearce-Kelly algorithm:

  • Maintains a mapping of keys to integer order values using StmContainers.Map
  • Implements addEdge to incrementally update order when new dependencies are added
  • Provides getAffectedKeysInOrder to efficiently sort keys by topological order

Updated Types.hs

Added TopoOrder data structure to SchedulerState:

  • Stores the topological order map using SMap.Map Key Int
  • Integrated into the scheduler state for global access

Updated Database.hs

  1. Initialize TopoOrder: Added initialization in newDatabase
  2. Maintain Order: Updated updateReverseDeps to call addEdge when dependencies change
  3. Use Order: Modified transitiveDirtyListBottomUpDiff to use getAffectedKeysInOrder for efficient sorting
  4. Documentation: Added detailed comments explaining the optimization

Performance Benefits

  • Before: O(V + E) DFS traversal + O(V log V) sorting on each invocation
  • After: O(V + E) DFS traversal + O(V) lookup and filter using pre-maintained order
  • Incremental Updates: Order is maintained incrementally during dependency changes (O(affected nodes))

The Pearce-Kelly algorithm ensures that the topological order is maintained with minimal overhead during dependency updates, making transitiveDirtyListBottomUpDiff faster especially for large dependency graphs.

Testing

  • All existing tests pass
  • Fixed test warnings by properly handling return values
  • The implementation is backward compatible and doesn't change any external APIs

Files Changed

  • hls-graph/hls-graph.cabal - Added new module to exposed modules
  • hls-graph/src/Development/IDE/Graph/Internal/Types.hs - Added TopoOrder data structure using StmContainers.Map
  • hls-graph/src/Development/IDE/Graph/Internal/TopoSort.hs - New module with Pearce-Kelly implementation, removed unused getOrderedList
  • hls-graph/src/Development/IDE/Graph/Internal/Database.hs - Integration of topological ordering
  • hls-graph/README.md - Documentation of the optimization
  • hls-graph/test/DatabaseSpec.hs - Fixed test warnings
Original prompt

transitiveDirtyListBottomUpDiff is slow, can you use Pearce–Kelly algorithm to keep a running topo of dependency graph so we can speed up transitiveDirtyListBottomUpDiff
transitiveDirtyListBottomUpDiff is slow, can you use Pearce–Kelly algorithm to keep a running topo of dependency graph so we can speed up transitiveDirtyListBottomUpDiff

Created from VS Code via the [GitHub Pull Request](https://marketplace.visualstudio.com/items?itemName=GitHub.vscode-pull-request-github) extension.

💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

@Copilot Copilot AI changed the title [WIP] transitiveDirtyListBottomUpDiff is slow, can you use Pearce–Kelly algorithm to keep a running topo of dependency graph so we can speed up transitiveDirtyListBottomUpDiff Optimize transitiveDirtyListBottomUpDiff using Pearce-Kelly algorithm for incremental topological ordering Oct 6, 2025

-- | Get all keys sorted by their topological order
getOrderedList :: TopoOrder -> STM [Key]
getOrderedList TopoOrder{..} = do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getOrderedList is O(n log n), not good, it's better to keep an updated ordered list instead of computing it right at time. @copilot

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed getOrderedList as it was not used anywhere and had O(n log n) complexity. The function is now eliminated from the API. (commit 6fb2e55)

-- | Topological ordering structure for Pearce-Kelly algorithm
-- Maps each Key to its topological order number (smaller = earlier in order)
data TopoOrder = TopoOrder
{ topoOrderMap :: !(TVar (Map.HashMap Key Int))
Copy link
Owner

@soulomoon soulomoon Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you use StmContainers.Map here? @copilot

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed TopoOrder to use StmContainers.Map instead of TVar (Map.HashMap ...). This is consistent with the rest of the codebase and provides better STM performance. (commit 6fb2e55)

@Copilot Copilot AI requested a review from soulomoon October 6, 2025 17:13
Copilot finished work on behalf of soulomoon October 6, 2025 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants